-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DEP: Deprecate fastCopyAndTranspose #22313
Conversation
I suspect the cause is that EDIT:
and
|
Having a "fast copy and transpose" operation in NumPy (presumably via a blocked iterator) would be amazing. And in principle we could ship some custom stuff inside this function. But my feeling would be that this is OK to deprecate because |
numpy/core/numeric.py
Outdated
and transpose methods instead, e.g. ``arr.copy().T`` | ||
""" | ||
warnings.warn( | ||
"fastCopyAndTranspose is deprecated. Use ``arr.copy().T`` instead", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"fastCopyAndTranspose is deprecated. Use ``arr.copy().T`` instead", | |
"fastCopyAndTranspose is deprecated. Use ``arr.T.copy()`` instead", |
You might also want to mention the "order" argument?
It desn't need much (not C code), but it would be nice to test that the warning is given. One option: Just keep the old tests and add with pytest.warns(DeprecationWarning):
, rather than adding a new test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option: Just keep the old tests and add with pytest.warns(DeprecationWarning):, rather than adding a new test.
The reason I did it this way was that the "private" _fastCopyAndTranspose
function is still used, so I reorganized the tests so that the private function is tested instead. I will add back the tests for the public function w/ the warnings catcher, as you're right that those tests should only be removed when the deprecation expires!
Also 👍 for the suggestion - I need to add it to the deprecation directive in the docs too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay the deprecation message is updated in 7b43315 and the tests of the public fn (with the deprecation check) are added back in bd9236d.
I didn't know how best to mention the order
kwarg without going into too much detail for a DeprecationWarning, so I simply went with order='K'
- LMK what you think!
Saw this by chance. The relevant speed comparison then is:
So, currently there is no difference at least for this size array (nor with an array a factor 100 larger). |
Heh :) |
Are you sure about the
|
Nope, I'm not sure at all :) I assumed users might expect the transposed memory layout, but I'm not sure. Ideally we could just say something like "use |
Well, it was only used in one place that we know of, so I suppose we really only need to worry about that. Since the deprecation is for the original, it would probably be best to keep the original C order for the replacement suggestion. |
Yeah, lets just keep it without any order (which is C in this case). My thought was we could just write |
Sorry, should have looked closer earlier :/. Should we just deprecate this in C? And with that I mean not just move the Deprecation there but also put it into Even in C, the implementation can just call Transpose and then copy (transpose can be passed If it is in C, it would be nice to have a very brief test in Also realized that it should have a release to check all boxes probably. |
I'm happy to put this in as is, the C function can be removed after the deprecation. The only worry is if someone is actually using the C function, and I am doubtful of that. The C function may listed as public, but the underscore suggests that it is private. |
Yeah, both routes seem good. I had briefly checked scipy (doesn't use the C version) and the C version is not documented at all, and also replaced with ~5 lines of code |
Release note needed. Deprecating the C version seems reasonable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This latest iteration is based on the approach discussed at the triage meeting:
- Remove the internal
_fastCopyAndTranspose
function now to get read of any aliasing/wrapping of the private Python function.fastCopyAndTranspose
now directly wraps the underlying C function. - ... which means there only needs to be one warning at the C level, which warns whenever a user calls either the Python (
np.fastCopyAndTranspose
) or C (PyArray_CopyAndTranspose
) functions. - Other minor fixups like using
DEPRECATE
instead ofPyErr_WarnFormat
for better grepability
This PR definitely needs to be squashed, as it's really 3 separate deprecation attempts in 1. I wanted to preserve the full history to provide context for the discussion on GH. If this latest changeset looks good, LMK and I'm happy to squash this down.
Deprecate the fastCopyAndTranspose function from the Python API, and the underlying PyArray_CopyAndTranspose function from the C-API. Also removes an internal, private function _fastCopyAndTranspose which was the original Python wrapper around the C-function.
2c442b6
to
06c380c
Compare
Thanks @rossbar |
I propose to deprecate the
fastCopyAndTranspose
function, which is currently accessible in the top-levelnumpy
namespace.Note that
fastCopyAndTranspose
was (prior to this PR) a public name bound to a private functionmultiarray._fastCopyAndTranspose
. There was a single usage of the function inlinalg
, but it was straightforward to reorganize things to use the private function instead. It's worth investigating whether the private function is even needed1, but I'm inclined to leave that for a separate PR and just focus on the deprecation to start.Re: downstream impact --- I grepped through some of the bigger projects in the ecosystem (scipy, matplotlib, scikit-learn, & astropy) and didn't find any instances of
fastCopyAndTranspose
. I'm happy to do a more thorough search if desired.Footnotes
rudimentary local benchmarking showed chaining the copy/transpose methods (i.e.
a.copy().T
) was 3x faster than_fastCopyAndTranspose
. ↩